Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update linter and add auto-formatter config #1314

Merged
merged 7 commits into from
Mar 19, 2021

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Mar 12, 2021

Description of the changes being introduced by the pull request:

Configure stricter linting, and enable and require auto-formatting to reduce future reviewing efforts for new code as per ADR3 in tuf/api/*. More specifically, this PR:

  • updates tuf/api/pylintrc,
  • configures tox to require code to be formatted by black (overall code style) and isort (order of import statements),
  • adds pre-commit configuration, which, upon optional local enabling, automatically runs black, isort and two basic builtin formatters on each git commit.

This PR further updates tuf/api/* source code using the added auto-formatters.
(If desired I can split commits by change and/or configure git for this repo to ignore the corresponding revision(s) in git-blame).

See commit messages for details.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Use black and isort to reformat new code in tuf/api/*, like so:

```
black --line-length 80 api
isort --line-length 80 --profile black api
```

Besides downsizing the default line length to fit our Code Style
Guide no extra configuration is required.

Unified format according to black and isort will be enforced by
CI/CD in a future commit.

**Changes include:**
- Use double quotes instead of single quotes where feasible
- Re-wrap and re-indent long lines such as dict literals, function
  signatures and function calls, using hanging indent
  This will require an update in our Code Style Guide, which the
  benefits of using black seem worth.
  https://github.com/secure-systems-lab/code-style-guidelines/blob/master/python.md#indentation-and-line-continuation
- Update vertical and horizontal spacing
- Sort and wrap imports

See black and isort docs for details:
https://black.readthedocs.io/en/stable/the_black_code_style.html
https://pycqa.github.io/isort/docs/configuration/black_compatibility/

NOTE: If desired I can split commits by change and/or configure git
for this repo to ignore the corresponding revision(s) in git-blame.
https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
The updated pylintrc is based on the Google Python Style Guide
pylint configuration at
https://google.github.io/styleguide/pylintrc with the following
differences:
- We don't list defaults which are applied anyway.
- We don't configure checks that seem unrelated to the code style
  guide.
- We don't disable any checks that are not in conflict with the
  current code or code style guide.

This has the advantage of a minimal configuration file which should
be easy to maintain and extend as required, e.g. if conflicting
code is added, or linting time becomes too long, due to unnecessary
checks.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Configure lint build in tox.ini to check if code in tuf/api/* is
formatted according to black and isort style rules:
https://black.readthedocs.io/en/stable/the_black_code_style.html
https://pycqa.github.io/isort/

In addition to our new style guide (theupdateframework#1128) and corresponding linter
configuration, requiring auto-formatting should help to further
reduce reviewing effort. The auto-formatter black was chosen for
the following reasons:
- It seems to be the most popular formatter in the Python ecosystem
- It is well documented including integration instructions with
  most of the tools we use (git, GitHub Actions, pylint, a range of
  editors, pyproject.toml theupdateframework#1161)
- It checks that the reformatted code produces a valid AST that is
  equivalent to the original
- It has almost no ways of customization, which means no
  customization effort required, and more (cross-project) style
  uniformity, lowering contribution barriers
- It converts single to double quotes, where reasonable, which is
  exactly what we recommend
- The style choices it makes seem generally reasonable and don't
 conflict with our style guide, except for favoring hanging over
 aligned indentation, which is the opposite of what we recommend.
 But we are willing to update the adapt our style guide.

Auto-format pre-commit configuration will be added in a subsequent
commit.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Add optional pre-commit configuration to install and run
auto-formatters when committing new code to tuf/api/*.
Auto-formatters include:
- trailing-whitespace
- end-of-file-fixer
- black
- isort

This commit also adds pre-commit to the dev dependencies
and updates the contributor instructions accordingly.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
files: ^tuf/api/
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.4.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider auto-updating the hooks in this file. Unfortunately, dependabot does not plan to support this (see dependabot/dependabot-core#1524). But there's a Github Action based workaround. Or we could use pre-commit's own ci -- pre-commit.ci. Although I lean towards not adding another CI/CD platform.

Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with using black as a formatter.
Your arguments make sense.

We would invoke black with:
black --line-length 80 api right?
Because we use 80 characters per line instead of 88.

and `isort <https://pycqa.github.io/isort>`_. The tasks can be installed as
`pre-commit <https://pre-commit.com/>`_ git hooks with the following command.
::
$ pre-commit install
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you clone tuf you will have the .pre-commit-config.yaml locally right?
Then, what does this command do exactly?

@lukpueh
Copy link
Member Author

lukpueh commented Mar 16, 2021

Thanks for the review, @MVrachev! Regarding your comments/questions...

I agree with using black as a formatter. Your arguments make sense.

🎉

When you clone tuf you will have the .pre-commit-config.yaml locally right?

Yes!

Then, what does this command [pre-commit install] do exactly?

It installs a script to a local .git/hooks/pre-commit file which will run the hooks (black, isort, etc.) specified in .pre-commit-config.yaml whenever you run git commit.

I recommend taking a quick look at the pre-commit documentation it's really a cool tool built on top of git.

We would invoke black with: black --line-length 80 api right? Because we use 80 characters per line instead of 88.

Yes, black needs to be invoked with --line-length 80, as you can see in tox.ini and .pre-commit-config.yaml. Once we have a pyproject.toml file (see #1161) we can configure this there for all uses of black in the project, be it tox, pre-commit, or you on the command line.

Also note that there's no reason to run black from the command line if you install the pre-commit hook.

@MVrachev
Copy link
Collaborator

MVrachev commented Mar 16, 2021

I would suggest adding documentation on how to run black from the command line as an addition to the git pre-commit hooks.

Sometimes you would want to run black through your command line before you commit your changes.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks @lukpueh. I am looking forward to not worrying about coding style any more.I fully agree that changing our coding style (hanging indents) to match black's style is the right choice, both because this gives contributors a more consistent style across Python projects, and because it minimises how much attention contributors have to pay to formatting (by automating it).

The one piece of this PR I am less enthusiastic about is providing a pre-commit configuration. I'm not completely against it, but it's one extra thing for us to maintain and something we have to tell contributors to enable. I wonder whether we might be better off updating docs/CONTRIBUTORS.rst to recommend contributors configure their editor to use black (linking to the black docs) or use pre-commit (and include a snippet like the .pre-commit-config.yaml in this PR) and then not include the file in our repository (avoiding the need for us to automate keeping it up-to-date).

deserializer: Optional[MetadataDeserializer] = None,
storage_backend: Optional[StorageBackendInterface] = None
) -> 'Metadata':
storage_backend: Optional[StorageBackendInterface] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, trailing commas in function signatures caught me off guard 🤯
The rationale in PEP 8, and black's documentation, seems reasonable. I am glad we will have a tool to automatically do this, though.

f'{len(signatures_for_keyid)} signatures for key '
f'{key["keyid"]}, not sure which one to verify.')
f"{len(signatures_for_keyid)} signatures for key "
f'{key["keyid"]}, not sure which one to verify.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This f-string (and the one above on ln269) looks a bit out of place, I wonder whether we should manually change these to f"dict['key']" - would black change those back?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted! I missed this when I prepared the PR. Looks like black doesn't change double quotes to single quotes nor adds escape characters, as a consequence it can't change the outer single quotes to double quotes. Unfortunately, pylint's quote consistency check also doesn't detect this, so I fear the onus will remain on the reviewer here. At least black does not change it back.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in be0cef0.

Black standardizes single to double quotes where feasible.
However, it doesn't seem to change double to single quotes nor adds
escape characters, as a consequence it skips standardization on
strings with mixed quotes.

Unfortunately, pylint's quote consistency check also doesn't detect
this, so the onus will remain on the reviewer in these cases.

**Unrelated changes**:
The commit still enables pylint's "check-quote-consistency" just in
case it can detect something the black doesn't.

The commit also fixes a syntax inconsistency in pylintrc.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
This reverts commit "Add basic pre-commit configuration for
tuf/api/*" (44aea45) in order to
reduce maintenance burdern:

- pre-commit really is a package manager, thus the packages (git
hooks) pulled in via pre-commit would need to be kept up-to-date
and securely so (sic!).

- pre-commit requires contributors to opt-in via "pre-commit
install" regardless, so we might as well ask contributors to add
and tend to the corresponding configuration file on their own.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@lukpueh lukpueh mentioned this pull request Mar 17, 2021
3 tasks
@lukpueh
Copy link
Member Author

lukpueh commented Mar 17, 2021

Ping @MVrachev and @joshuagl, this is ready for a re-review, whenever you have time! 🙏

@lukpueh
Copy link
Member Author

lukpueh commented Mar 17, 2021

I would suggest adding documentation on how to run black from the command line as an addition to the git pre-commit hooks.

Fixed in 2e883d4

@lukpueh
Copy link
Member Author

lukpueh commented Mar 17, 2021

The one piece of this PR I am less enthusiastic about is providing a pre-commit configuration. I'm not completely against it, but it's one extra thing for us to maintain and something we have to tell contributors to enable. I wonder whether we might be better off updating docs/CONTRIBUTORS.rst to recommend contributors configure their editor to use black (linking to the black docs) or use pre-commit (and include a snippet like the .pre-commit-config.yaml in this PR) and then not include the file in our repository (avoiding the need for us to automate keeping it up-to-date).

Fixed in 38ef45f.

or via source code editor plugin
[`black <https://black.readthedocs.io/en/stable/editor_integration.html>`__,
`isort <https://github.com/pycqa/isort/wiki/isort-Plugins>`__] or
`pre-commit <https://pre-commit.com/>`__-powerd git hooks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`pre-commit <https://pre-commit.com/>`__-powerd git hooks
`pre-commit <https://pre-commit.com/>`__-powered git hooks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and force-pushed!

Add cli snippet to run black and isort on the command line and
pointers to editor and pre-commit configuration to
docs/CONTRIBUTORS.rst.

Also add .pre-commit-config.yaml to .gitignore for independent
pre-commit configuration.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks @lukpueh!

I will hold off on merging to give @MVrachev a chance to re-review.

@lukpueh
Copy link
Member Author

lukpueh commented Mar 18, 2021

Let's also land #1261 first for easier conflict resolution.

@lukpueh lukpueh mentioned this pull request Mar 19, 2021
17 tasks
Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, thanks @lukpueh!

I will hold off on merging to give @MVrachev a chance to re-review.

Thank you, Joshua!
LGTM!

@lukpueh
Copy link
Member Author

lukpueh commented Mar 19, 2021

Thanks again for the reviews, @joshuagl and @MVrachev! Merging...

@lukpueh lukpueh merged commit c2b1f0e into theupdateframework:develop Mar 19, 2021
@lukpueh
Copy link
Member Author

lukpueh commented Mar 19, 2021

Oh no. Silly me. Forgot about #1261. 🤦

@lukpueh
Copy link
Member Author

lukpueh commented Mar 19, 2021

@jku, I'm happy to help rebasing.

@jku
Copy link
Member

jku commented Mar 19, 2021

Sorry I've been distracted: thanks for the offer but I'm just about to do the rebase now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants